Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(issue-views): Change add view button to just + icon near starred views header' #87360

Merged
merged 4 commits into from
Mar 19, 2025

Conversation

MichaelSun48
Copy link
Member

@MichaelSun48 MichaelSun48 commented Mar 18, 2025

Moves the Add View Button from below the views to the right of the "Starred Views" header. Adds a tooltip that says "Add View"

Before:

image

After:

image

Includes a couple of annotated drive by changes that aren't super related

@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Mar 18, 2025
Comment on lines +95 to +96
height: 14px !important;
margin: 0 !important;
Copy link
Member Author

@MichaelSun48 MichaelSun48 Mar 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Idk why but there are top level sentry.css styles that are forcing margin and height properties on the loadingindicator.mini class.

image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh I hate when we run into high specificity CSS styles

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if adding mini is conflicting with the height prop you're sending?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might, but mini also controls the width of the loading ring. Without it here, the width looks too big relative to the space its contained in.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah that makes sense

@@ -149,12 +151,13 @@ export function IssueViewNavItemContent({
dragListener={false}
dragControls={controls}
style={{
...(isDragging
...(isDragging || scrollPosition === 0
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This kinda fixes that really annoying issue where views wouldn't animate in and out as a result of the originY styles. We only need to apply those styles if the user has scrolled down on the page, since that's what triggers the framer motion bug.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice to add a comment here about why this is necessary since it will be very confusing to future readers!

position: relative;
background-color: ${p => p.theme.translucentSurface200};
background-color: ${p => (p.grabbing ? p.theme.translucentSurface200 : 'transparent')};
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Views had a slightly different background in their resting state. This applies the translucent border only when dragging.
image

Comment on lines +262 to 269
<SecondaryNav.Section
title={
<TitleWrapper>
{t('Starred Views')}
<IssueViewAddViewButton baseUrl={baseUrl} />
</TitleWrapper>
}
>
Copy link
Member Author

@MichaelSun48 MichaelSun48 Mar 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is moved in from navigation.tsx

@MichaelSun48 MichaelSun48 marked this pull request as ready for review March 19, 2025 16:59
@MichaelSun48 MichaelSun48 requested a review from a team as a code owner March 19, 2025 16:59
Copy link
Member

@malwilley malwilley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple suggestions but looks good!

@MichaelSun48 MichaelSun48 merged commit 2367fa8 into master Mar 19, 2025
41 checks passed
@MichaelSun48 MichaelSun48 deleted the msun/issueViews/moveAddViewButton branch March 19, 2025 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Scope: Frontend Automatically applied to PRs that change frontend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants